There are a number of interfaces here that could be removed.

Comment on attachment 520984 [details] [diff] [review] Moved the methods GetRowSpec and GetColSpec into class nsHTMLFrameSetElement and added a headerfile for it. This is a little hard to make sense of. It looks like your editor is converting to Windows line endings. If you can attach a diff with the comments addressed and Unix line endings this will be much better. >+ * Sebastian Kromp <46b@gulli.com> Don't add your name to nsHTMLFrameSetElement.cpp, add it to the .h. >+/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */ >+/* ***** BEGIN LICENSE BLOCK ***** >+ * Version: MPL 1.1/GPL 2.0/LGPL 2.1 >+ * >+ * The contents of this file are subject to the Mozilla Public License Version >+ * 1.1 (the "License"); you may not use this file except in compliance with >+ * the License. You may obtain a copy of the License at >+ * http://www.mozilla.org/MPL/ >+ * >+ * Software distributed under the License is distributed on an "AS IS" basis, >+ * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License >+ * for the specific language governing rights and limitations under the >+ * License. >+ * >+ * The Original Code is Mozilla Communicator client code. >+ * >+ * The Initial Developer of the Original Code is >+ * Netscape Communications Corporation. >+ * Portions created by the Initial Developer are Copyright (C) 1998 >+ * the Initial Developer. All Rights Reserved. >+ * >+ * Contributor(s): >+ * >+ * Alternatively, the contents of this file may be used under the terms of >+ * either of the GNU General Public License Version 2 or later (the "GPL"), >+ * or the GNU Lesser General Public License Version 2.1 or later (the "LGPL"), >+ * in which case the provisions of the GPL or the LGPL are applicable instead >+ * of those above. If you wish to allow use of your version of this file only >+ * under the terms of either the GPL or the LGPL, and not to allow others to >+ * use your version of this file under the terms of the MPL, indicate your >+ * decision by deleting the provisions above and replace them with the notice >+ * and other provisions required by the GPL or the LGPL. If you do not delete >+ * the provisions above, a recipient may use your version of this file under >+ * the terms of any one of the MPL, the GPL or the LGPL. >+ * >+ * ***** END LICENSE BLOCK ***** */ >+ It looks like you have two license blocks inside nsHTMLFrameSetElement.cpp. Remove the second one. >diff -r 290712e55ade content/html/content/src/nsHTMLFrameSetElement.h >+ * ***** END LICENSE BLOCK ***** */ >+#ifndef nsHTMLFrameSetElement_h__ >+#define nsHTMLFrameSetElement_h__ Newline between the comments and the macros please. >+#endif nsHTMLFrameSetElement_h__ >+ >+ Only one newline at the end of the file. Other than it looks good. Please fix the newlines and the comments and attach a new diff.

Please use nsHTMLFrameSetElement_h instead of nsHTMLFrameSetElement_h__ for the header guards. Identifiers with two consecutive underscores are reserved.

Comment on attachment 521508 [details] [diff] [review] Moved the methods GetRowSpec and GetColSpec into class nsHTMLFrameSetElement and added a headerfile for it.(fixed) >+ static nsHTMLFrameSetElement* FromContent(nsIContent *aContent) >+ { >+ if (aContent->NodeInfo()->Equals(nsGkAtoms::option, kNameSpaceID_XHTML)) >+ return static_cast<nsHTMLFrameSetElement*>(aContent); >+ return nsnull; >+ } Ah, now that I can see, there's one problem here. This should be nsGkAtoms::frameset, since this is a frameset element. Other than that it looks good. Fix that, attach a new patch, and request review from ":bz".

Can you please not make all those whitespace changes? That will make this patch much simpler to review and not mess up the blame.... If we do want to make the whitespace changes, that should be done in a separate patch with no substantive changes in it.

Sorry for that, didn´t mentioned that it was that messed up. I hope it is okay now.

Comment on attachment 523008 [details] [diff] [review] Moved the methods GetRowSpec and GetColSpec into class nsHTMLFrameSetElement and added a headerfile for it.(fixed whitespaces) >+++ b/content/html/content/src/nsHTMLFrameSetElement.h Wed Mar 30 16:32:52 2011 +0200 It looks like you removed the old header and added the new one. You should probably change things so that you hg move the old header to the new name and then edit it. That would keep revision history better. >+ if (aContent->NodeInfo()->Equals(nsGkAtoms::frameset, kNameSpaceID_XHTML)) if (aContent->IsHTML(nsGkAtoms::frameset)) >+++ b/layout/generic/nsFrameSetFrame.h Wed Mar 30 16:32:52 2011 +0200 >+#include "nsHTMLFrameSetElement.h" Why is this include needed at all? Just including that header in nsFrameSetFrame.cpp should be enough.

Okay, had fixed all that you mentioned. Just out of curiosity: why is it better to include it in the *.cpp instead of the header?

Comment on attachment 527859 [details] [diff] [review] Moved the methods GetRowSpec and GetColSpec into class nsHTMLFrameSetElement and added a headerfile for it. > why is it better to include it in the *.cpp instead of the header? Fewer chances of include hell if someone else includes the header. Please restore the comments about GetRowSpec and GetColSpec that the interface used to have and remove the "// nsIFramesetElement" comment. r=me with those changes, and sorry for the terrible lag. :( If you want me to just make those updates and push this, please let me know, ok? Otherwise, please set the checkin-needed keyword once you post an updated patch.

This patch added a build warning due to the text after #endif here: > #endif nsHTMLFrameSetElement_h "nsHTMLFrameSetElement_h" wants to be a comment there. I pushed a trivial followup to fix that: http://hg.mozilla.org/mozilla-central/rev/79394c301944

Is there a way this fix can be verified by QA? If so, please help me with some test case/STR/guidelines. Thank you